-
Notifications
You must be signed in to change notification settings - Fork 682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(bazel) Set @platforms//os:emscripten for platform_wasm #1363
(bazel) Set @platforms//os:emscripten for platform_wasm #1363
Conversation
This change require use platforms in version 0.0.9+, in my own project, I set it in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much!
CI is reporting I guess something else needs updating? |
Likely a bazel version bump is needed somewhere... |
If I understand the operation of bazel correctly, bazel has a built-in dependency for |
Sadly I'm not entirely sure either. I suspect our bazel on CI is out of date. I think that's controlled approximately here: https://github.com/emscripten-core/emsdk/blob/main/.circleci/config.yml. It looks like windows is using bazelisk but requesting version 5.4.0, which appears to be pretty old. It looks like mac is also using bazelisk but not requesting a version, so I'd sort of expect that one to work. And then linux... I'm not really sure what's going on with that. What version of bazel are you running locally? @trzeciak |
Since essentially everything depends on With Bzlmod, bumping the All in all, I would say go for the update. |
I don't think there is a question as to whether we want to upgrade, but more of question of how to fix the CI tests which are failing. We can't land this until we fix those. |
Could you try adding maybe(
http_archive,
name = "platforms",
urls = [
"https://mirror.bazel.build/github.com/bazelbuild/platforms/releases/download/0.0.9/platforms-0.0.9.tar.gz",
"https://github.com/bazelbuild/platforms/releases/download/0.0.9/platforms-0.0.9.tar.gz",
],
sha256 = "5eda539c841265031c2f82d8ae7a3a6490bd62176e0c038fc469eabf91f6149b",
) to this macro? |
Oooo, this looks very promising, I will try it locally first. |
@fmeum Ok, I test it locally on macOS, and after add your snippet to deps macro, I tested it for few different version of bazel (using bazelisk):
and it seems that not work for 7+ version. I also followed the Following this line of reasoning, I suppose I can add [email protected] dependency in a new MODULE file in this repo against to WORKSPACE (I will try this way). By the way, it makes sense that this doesn't work in Bazel 7, because the maybe method is described as adding a dependency if it doesn't exist at all: def maybe(repo_rule, name, **kwargs):
"""Utility function for only adding a repository if it's not already present.
… Going further, it might be nice to update emsdk to fully support bzlmod, but maybe not in this MR. |
Head branch was pushed to by a user without write access
Bazel 7+ additionally requires `platforms` dependencies in the `MODULE.bazel` file. | ||
```starlark | ||
bazel_dep(name = "platforms", version = "0.0.9") | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bazel 7+ additionally requires `platforms` dependencies in the `MODULE.bazel` file. | |
```starlark | |
bazel_dep(name = "platforms", version = "0.0.9") | |
``` |
This isn't needed: Bzlmod resolves transitive dependencies via Minimum Version Selection, so adding the platforms
dep to the emsdk's MODULE.bazel
file is sufficient for dependent modules to pick it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that you are talking about Bazel 7+, but versions 5.4 and/or 6x do not check the MODULE file (by default), so a macro update is needed for older versions.
Should we be concerned about Bazel 5x/6x users? → I have no idea 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, this snippet is meant for users pulling in emsdk
via WORKSPACE but who do have --enable_bzlmod
set, e.g. because it is the default in Bazel 7+? That makes sense if you don't want to fully set up emsdk
for Bzlmod.
If you do, then this wouldn't be necessary and such users could just load emsdk
via a bazel_dep
in MODULE.bazel instead. But that could be done in a separate PR.
@@ -0,0 +1 @@ | |||
bazel_dep(name = "platforms", version = "0.0.9") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To properly Bzlmod-ify this so that other Bazel projects can depend on it, you also need to add something like this to the top:
module(
name = "emsdk",
version = "<current version>",
)
Instead of manually maintaining <current version>
, you can also use the Publish to BCR app to automate the version bump and releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope I understand what you mean xD (I'm still new in bazel).
If it's not a problem, I'll try to do it in the next/separate MR.
Thanks for the tip!
Thanks very much! Feel free to send further PRs my way. |
I noticed that @googlewalt added
@platforms//os:emscripten
LINK, now we can update theplatform_wasm
.